-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
records: update json schema to include container images #3391
Conversation
"items": { | ||
"properties": { | ||
"description": { | ||
"description": "A brief description of the container image", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be useful to specify that the image is used for analysing the data, for example:
- Description of the container image that is recommended for analysing these data
(Alternatively, if we could have several images linked here, one recommended for analysis, one how the data was born, we could even introduce a new property called "purpose" or some such... But at this moment it's probably an overkill, since we plan to store only the images useful for data analyses. Unless there could be several images in the future for different purposes... CC @katilp )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for now, we do not have plans to link to other images than that to be used for analysing the data.
(I could only imagine a separate image for MC to generate a new similar MC dataset with some changes in the input but we are not providing those images and I'd rather not link them to the records in any case)
"type": "string" | ||
}, | ||
"recid": { | ||
"description": "The internal ID of the container image, if it is another Open Data record", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start again by the same word as the field is called (recid
) so that readers can form better mental association:
- Record ID of the container image (if it exists as another Open Data record)
"type": "string" | ||
}, | ||
"registry": { | ||
"description": "The type of registry the image can be found at", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be useful to provide example values in the documentation:
- Registry type where the image can be found (e.g. dockerhub, cerngitlab)
"type": "string" | ||
}, | ||
"uri": { | ||
"description": "The URI for finding the container image", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again here:
- URI where the container image is located
However, we may perhaps be better off using a common term FQIN here to denote fully-qualified image name concept, i.e. the field would be called "fqin" instead of "uri" and the description would explain it as follows:
- Fully Qualified Image Name (FQIN) where the container image is located (repository/name:tag)
We can discuss live also with @katilp before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. BTW it the "fqin" field name risks looking a bit weird, we could use some longer name variant such as "fully_qualified_name", or just "name", and document that the name should be FQIN in the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points! I would go for "name" and documenting it in the schema as @tiborsimko proposed
b136490
to
e01bdfe
Compare
e01bdfe
to
c3707a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, rebased against latest QA.
After merge, please prepare another PR with a small example file containing the data as well as the output format Jinja snippet displaying it, as we had talked about IRL.
fixes #3386
updated records' JSON schema to include container images metadata fields